Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editorial: Refactor PartitionPattern for simplicity and comprehensibility #840

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

gibson042
Copy link
Contributor

@gibson042 gibson042 commented Nov 14, 2023

  • Adds a prose description explaining treatment of brace-wrapped placeholders and adjacent literal text in converting a String pattern into an equivalent List of Records.
  • Renames spec aliases (beginIndexplaceholderStart, endIndexplaceholderEnd, pplaceholderName).
  • Adopts ECMA-262 phrasing conventions (e.g., "the substring of S from inclusiveStart [to exclusiveEnd]" and <///> operators).
  • Reduces unnecessary repetition (e.g., "If beginIndex > nextIndex, then Let literal be the substring of pattern from nextIndex to beginIndex and Append the Record { [[Type]]: "literal", [[Value]]: literal } to result." → "Let literal be the substring of pattern from placeholderEnd + 1 to placeholderStart and If literal is not the empty String, then Append the Record { [[Type]]: "literal", [[Value]]: literal } to result.").
  • Makes more obvious the omission of zero-length literal Records.
  • Increases alias reference locality (e.g., checking for a literal part between a previous placeholderEnd } and current placeholderStart { before advancing placeholderEnd for the current placeholder part).

@gibson042 gibson042 requested a review from ryzokuken November 14, 2023 06:02
@gibson042 gibson042 force-pushed the 2023-11-clarify-partitionpattern branch 2 times, most recently from 449b5a2 to f9abfa8 Compare November 14, 2023 19:47
@gibson042
Copy link
Contributor Author

@ryzokuken ping

@ryzokuken ryzokuken added editorial Involves an editorial fix needs review labels Feb 22, 2024
@gibson042 gibson042 requested a review from ben-allen March 1, 2024 04:44
Copy link
Contributor

@ben-allen ben-allen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! the spec steps are much clearer now, and the new description is something I wish had been in 402 when I started reading it.

A couple of ideas that I'm not sure are good or not:

  1. I realized while reviewing this that the word "placeholder" isn't actually used that often in 402 -- twice in the NumberFormat spec and nowhere else -- and thought it might be worthwhile to provide a definition. I searched around a little for a link to a definition (thinking I'd find it in documentation for CLDR or ICU) but without much success. Would it actually be useful to define the term, either in the text or a link to a relevant definition, or can we assume that anyone who's reading it is either already familiar or can easily figure it out from context?
  2. The description refers to placeholders as placeholders in the first clause, and then later as substrings, i.e. brace-wrapped placeholders become represented as unwrapped substrings. To my eye the switch between one word and the other carries an implication that the placeholder and the substring that's yielded when you remove the braces are different things. Starting the description with something like "It represents each brace-wrapped substring ("placeholder") in pattern" could be a way to both clarify the sentence and also define the term.

@gibson042 gibson042 force-pushed the 2023-11-clarify-partitionpattern branch from f9abfa8 to bb74c59 Compare April 29, 2024 15:21
@gibson042 gibson042 merged commit 578aaa5 into tc39:main Apr 29, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial Involves an editorial fix needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants